fix(zigbee2mqtt): prefer device friendly name for topic and handle keypad action vocabulary - #700
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
==========================================
+ Coverage 84.14% 93.97% +9.82%
==========================================
Files 10 42 +32
Lines 801 5407 +4606
Branches 0 30 +30
==========================================
+ Hits 674 5081 +4407
- Misses 127 326 +199
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
tykeal
left a comment
tykeal
left a comment
There was a problem hiding this comment.
Walkthrough
Reworks Zigbee2MQTTLockProvider.base_topic to derive the MQTT topic from device_entry.name_by_user / device_entry.name, keeping the zigbee2mqtt_<id> identifier suffix only as a fallback, and broadens _async_handle_action to accept bare lock/unlock actions when the payload marks the source as the keypad. Adds four unit tests. Addresses bugs 1 and 2 of #699.
Changes
custom_components/keymaster/providers/zigbee2mqtt.py:base_topicpreference order inverted (name_by_user > name > identifier suffix, elseNone);_async_handle_actiongains apayloadargument and anis_keypaddiscriminator based onaction_source_name/action_source/keypad_prefix;handle_state_messagenow forwards the full payload.tests/providers/test_zigbee2mqtt.py: existing rename test inverted; four new tests for name preference,name_by_userpreference, identifier fallback, and two keypad action payloads.
Review Comments
Details inline. Cross-cutting concerns:
1. name_by_user is the wrong source of truth (blocker, inline). Renaming a device in the HA UI sets name_by_user and has no effect on the Z2M friendly name or the MQTT topic. Preferring it means any user who has renamed the lock device in HA gets a topic Z2M never publishes to — the exact class of failure #699 reports, reintroduced from the other direction. The pre-existing code comment ("to support device renaming") documented that this was deliberate. device_entry.name alone is the correct fix and matches the reporter's suggestion.
2. This PR does not make #699's reported symptom go away. Bugs 3 and 4 in that issue are in coordinator.py and are untouched:
_handle_provider_lock_event(coordinator.py:860-870) evaluatesstate_changedbefore the label semantics. Z2M publishesaction/action_userin the same payload as the post-operationstate, so at callback time the entity state has typically already flipped andstate_changedwins, routing "Unlocked via Keypad" wherever the entity state points rather than to the label's meaning._handle_lock_state_change(coordinator.py:934-950) never assignskmlock.lock_statefor push providers, and the provider event path bails onnot isinstance(slot_num, int)(Z2M manual/one-touch lock publishesaction_user: null).kmlock.lock_statetherefore drifts, and_lock_unlocked's early-return guard drops subsequent keypad unlocks.
With this PR alone, the topic and the vocabulary are fixed but Last Used may still not update. Either land the coordinator half too or state explicitly in the PR that #699 remains open. Note coordinator.py is being heavily edited by the open #695 — coordinate ordering there to avoid a conflicting fix.
3. Test coverage is happy-path only. Every new test asserts a positive match. Missing: the negative discriminator case (non-keypad unlock), the base_topic is None path, and any test that handle_state_message actually forwards the payload — the plumbing change is exercised nowhere; all four new tests call _async_handle_action directly. Inline suggestions below.
No security issues, blocking I/O, or PII-in-logs concerns found.
| # Prefer device_entry.name_by_user / device_entry.name if set to a string | ||
| if isinstance(device_entry.name_by_user, str) and device_entry.name_by_user: | ||
| return f"zigbee2mqtt/{device_entry.name_by_user}" | ||
| if isinstance(device_entry.name, str) and device_entry.name: | ||
| return f"zigbee2mqtt/{device_entry.name}" |
There was a problem hiding this comment.
[BLOCKER] name_by_user is HA-local and is set precisely when a user renames the device in the HA UI. That rename does not change the Z2M friendly_name, so the topic derived from it will not exist on the broker. Any install where the lock device was renamed in HA breaks after this change — writes keep working (Z2M accepts IEEE on /set), so it fails silently in exactly the way #699 describes.
Secondary: name_by_user is free-form user text and can contain + or #, which turn mqtt.async_subscribe into a wildcard subscription over unrelated devices. device_entry.name comes from Z2M discovery and is constrained by Z2M's own friendly-name validation.
| # Prefer device_entry.name_by_user / device_entry.name if set to a string | |
| if isinstance(device_entry.name_by_user, str) and device_entry.name_by_user: | |
| return f"zigbee2mqtt/{device_entry.name_by_user}" | |
| if isinstance(device_entry.name, str) and device_entry.name: | |
| return f"zigbee2mqtt/{device_entry.name}" | |
| # Prefer device_entry.name, which HA populates from the Z2M friendly | |
| # name via discovery. Do not use name_by_user: an HA-side rename does | |
| # not change the Z2M friendly name and therefore not the MQTT topic. | |
| if isinstance(device_entry.name, str) and device_entry.name: | |
| return f"zigbee2mqtt/{device_entry.name}" |
| action_source_name = payload.get("action_source_name") | ||
| action_source = payload.get("action_source") | ||
| is_keypad = ( | ||
| action_source_name == "keypad" | ||
| or action_source == 0 | ||
| or (isinstance(action, str) and action.startswith("keypad_")) | ||
| ) |
There was a problem hiding this comment.
[SUGGESTION] The discriminator is not defensive against payload variation across converters/firmware:
action_source == 0isTrueforaction_source: false(PythonFalse == 0), andFalsefor the string"0", which some converters emit.action_source_namecasing is converter-dependent; compare case-insensitively.
| action_source_name = payload.get("action_source_name") | |
| action_source = payload.get("action_source") | |
| is_keypad = ( | |
| action_source_name == "keypad" | |
| or action_source == 0 | |
| or (isinstance(action, str) and action.startswith("keypad_")) | |
| ) | |
| action_source_name = payload.get("action_source_name") | |
| action_source = payload.get("action_source") | |
| is_keypad = ( | |
| (isinstance(action_source_name, str) and action_source_name.lower() == "keypad") | |
| or ( | |
| isinstance(action_source, int) | |
| and not isinstance(action_source, bool) | |
| and action_source == 0 | |
| ) | |
| or (isinstance(action, str) and action.startswith("keypad_")) | |
| ) |
| async def test_base_topic_prefers_name_by_user(self, provider, mock_hass): | ||
| """Test that base_topic prefers device_entry.name_by_user when set.""" | ||
| setup_successful_connect( | ||
| provider, | ||
| mock_hass, | ||
| device_name="Front Door Lock", | ||
| identifiers={("mqtt", "zigbee2mqtt_0x000d6f001933df17")}, | ||
| ) | ||
| device_entry = provider.device_registry.async_get.return_value | ||
| device_entry.name_by_user = "Custom User Name" | ||
| assert provider.base_topic == "zigbee2mqtt/Custom User Name" |
There was a problem hiding this comment.
[BLOCKER] This test pins the name_by_user behaviour flagged above. It should be inverted: an HA-side rename must not change the topic.
| async def test_base_topic_prefers_name_by_user(self, provider, mock_hass): | |
| """Test that base_topic prefers device_entry.name_by_user when set.""" | |
| setup_successful_connect( | |
| provider, | |
| mock_hass, | |
| device_name="Front Door Lock", | |
| identifiers={("mqtt", "zigbee2mqtt_0x000d6f001933df17")}, | |
| ) | |
| device_entry = provider.device_registry.async_get.return_value | |
| device_entry.name_by_user = "Custom User Name" | |
| assert provider.base_topic == "zigbee2mqtt/Custom User Name" | |
| async def test_base_topic_ignores_name_by_user(self, provider, mock_hass): | |
| """Test that an HA-side rename (name_by_user) does not change the topic.""" | |
| setup_successful_connect( | |
| provider, | |
| mock_hass, | |
| device_name="Front Door Lock", | |
| identifiers={("mqtt", "zigbee2mqtt_0x000d6f001933df17")}, | |
| ) | |
| device_entry = provider.device_registry.async_get.return_value | |
| device_entry.name_by_user = "Custom User Name" | |
| assert provider.base_topic == "zigbee2mqtt/Front Door Lock" |
|
I withdraw the cross-cutting finding that #700 is incomplete because it fixes only bugs 1–2 of #699. Splitting #699 across #700 (Z2M topic/keypad action vocabulary), #701 (coordinator event-label intent/push-provider lock state sync), and #702 (lovelace strategy resource lifecycle) is the correct approach, and #701/#702 cover the remaining parts. The other findings still stand, in particular the |
|
Thanks for the review! I've pushed a fix in commit
Regarding
|
|
Resolved the
All 57 Z2M provider tests pass cleanly. |
tykeal
left a comment
tykeal
left a comment
There was a problem hiding this comment.
Both blockers are resolved. base_topic no longer consults name_by_user, and test_base_topic_ignores_name_by_user pins the corrected behaviour.
The precedence inversion is correct: Z2M publishes state only on zigbee2mqtt/<friendly_name>, HA populates device_entry.name from that friendly name via discovery, and an HA-side rename writes name_by_user instead. The IEEE identifier fallback still works for /set and /get, so it remains a sound last resort.
Withdrawing my earlier suggestion on the is_keypad discriminator — it was unfounded and should not be applied. action_source is a ZCL uint8 (fromZigbee.ts: action_source: msg.data.opereventsrc), so it is never false or "0", and constants.ts lockSourceName is fixed lowercase, so mixed-case action_source_name cannot occur.
The negative tests are load-bearing: removing the is_keypad gating fails test_async_handle_action_ignores_non_keypad_source, test_async_handle_action_ignores_bare_action_without_source and test_subscribe_lock_events_ignores_other_actions; removing the payload guard fails test_async_handle_action_with_non_dict_payload.
Merges cleanly against current main with no overlap with #695. 57 provider tests pass.
Three non-blocking notes, take or leave:
zigbee2mqtt.py:86— the comment describes the identifier suffix as a friendly name, but Z2M sets device identifiers from the IEEE address; the localfriendly_namevariable is misnamed for the same reason. Inherited wording, now inaccurate.tests/providers/test_zigbee2mqtt.py— step 2 oftest_connect_success_rename_deviceasserts the same value as step 1 now thatnamewins unconditionally, so it no longer tests what its comment claims.- Z2M supports a per-device
homeassistant: {name: ...}override, which makesdevice_entry.namediverge fromfriendly_name. Those installs get a topic that receives no state, with no fallback since thenamebranch wins. Rare against the common case this fixes, but worth knowing.
…est coverage for empty device name
|
Thanks @tykeal! Applied both non-blocking suggestions in
|
|
LGTM — approved. |
4 participants
Summary of Changes
base_topicderivation inZigbee2MQTTLockProviderto preferdevice_entry.name(the Z2M friendly name) over IEEE identifier parsing._async_handle_actionto inspectaction_source_name/action_sourcepayloads for keypad lock/unlock actions (e.g.action: "unlock"withaction_source_name: "keypad").tests/providers/test_zigbee2mqtt.py.Ref #699